Re-design Env of the global object init checker #24107
Conversation
| (using State.Data, EnvMap.EnvMapMutableData, Trace): EnvRef = | ||
| assert((sym.is(Flags.Param) && sym.info.isInstanceOf[ExprType]) || sym.is(Flags.Lazy)); | ||
| _of(Map.empty, tree, thisV, outerEnv) | ||
|
|
There was a problem hiding this comment.
I think this is OK, but is there a rationale for the inconsistency that we use the rhs as the tree to identify a lazy val but the overall DefDef as the tree to identify a method (as opposed to the rhs of the method)?
If there is a rationale, it would be good to record it in a comment.
There was a problem hiding this comment.
Documented the rationale. We must use the overall DefDef as the tree to identify a method (as opposed to the rhs of the method) because resolveEnvByMethod looks for the symbol of the DefDef to see it if matches the target method
| ref.valValue(target) | ||
| else if target.hasSource then | ||
| val rhs = target.defTree.asInstanceOf[ValDef].rhs | ||
| given Scope = Env.ofByName(target, rhs, ref, Env.NoEnv) |
There was a problem hiding this comment.
Why is the outerEnv NoEnv? Can't the rhs of a lazy val refer to things from the enclosing scope of its definition site?
There was a problem hiding this comment.
For lazy fields, their outer scope is a template and they can refer to other fields. However, currently we do not create environments that encapsulate states of instances, so the outerEnv is NoEnv for lazy fields. The values of other fields can be found by searching through thisV instead, and thisV is an InstanceRef with its fields and outer environments.
Another possible design is to store all outer environments as a chain in EnvMap and bypasses the InstanceRef, or even creates environments for instantiation. Do you prefer any of these designs? Maybe we can modify it in another PR
There was a problem hiding this comment.
OK, that makes sense. Thanks for the explanation. We can keep the current design.
| val byNameEnv = scope match { | ||
| case ref: Ref => Env.ofByName(sym, thisV, Env.NoEnv) | ||
| case env: Env.EnvRef => Env.ofByName(sym, thisV, Env.EnvSet(Set(env))) | ||
| case ref: Ref => Env.ofByName(sym, code, thisV, Env.NoEnv) // for by-name arguments of constructors |
| case class EnvRef(tree: Tree, owner: ClassSymbol)(using Trace) extends Scope: | ||
| def show(using Context) = | ||
| "meth: " + meth.show + "\n" + | ||
| "tree: " + tree.show + "\n" + |
There was a problem hiding this comment.
The document for the class EnvRef is outdated.
There was a problem hiding this comment.
Will there a problem with equality? The tree will have structural equality, while previously the symbol has reference equality.
There was a problem hiding this comment.
The document for the class
EnvRefis outdated.
Updated the document
There was a problem hiding this comment.
Will there a problem with equality? The tree will have structural equality, while previously the symbol has reference equality.
Defined the equals method, which checks reference equality of the trees
EnzeXing
force-pushed
the
fix-global-init-checker-resolve-env
branch
from
54205cf to
480a6d5
Compare
EnzeXing
force-pushed
the
fix-global-init-checker-resolve-env
branch
from
480a6d5 to
98d2ed5
Compare
When the global object init checker models lazy fields and by-name parameters, initially it creates an environment for each symbol of the lazy field/by-name parameter. For by-name parameters, the environment includes all the possible environments that pass to the parameter as outer environments. This PR modifies the design of environments to take the tree to evaluate as the argument. Therefore, the same of argument tree passed to a by-name parameter will share the same environment, and there will be multiple environments for the same by-name parameter. This facilitates the process of finding local variables in the argument tree. Now the process of evaluating by-name args in
multiple-by-name.scalawill behave similarly to evaluating closure arguments inmultiple-func-arg.scala